Code Review — feat!: spec sync 3.21.0 → 3.22.0 (v5.0.0)

Overview

This is a well-executed breaking-change release. The core operation — removing the V1 order-write surface now that Kalshi dropped it from the spec — is done surgically: every V1 method, model, import, test, contract entry, and EXCLUSION is cleaned up without touching adjacent code. The three new RFQ-scoped quote endpoints follow existing conventions exactly. Migration guide and CHANGELOG are comprehensive. Test suite passes (4097/0).

Issues / Concerns

1. cancel_v2 with market_ticker has no unit test (low severity)

The BatchCancelOrdersV2RequestOrder.exchange_index=-1, market_ticker=... path is tested (test_auto_route_exchange_index_minus_one), but the singular cancel_v2(order_id, exchange_index=-1, market_ticker="MKT-A") path has no corresponding test. Given that this is a spec-required pairing, worth a quick test:

@respx.mock
def test_cancel_v2_auto_route_sends_market_ticker(self, orders: OrdersResource) -> None:
    route = respx.delete(
        "https://test.kalshi.com/trade-api/v2/portfolio/events/orders/ord-1"
    ).mock(return_value=httpx.Response(200, json={...}))
    orders.cancel_v2("ord-1", exchange_index=-1, market_ticker="MKT-A")
    assert route.calls[0].request.url.params["market_ticker"] == "MKT-A"

2. exchange_index=-1 + missing market_ticker goes unvalidated (low severity)

Both cancel_v2 and BatchCancelOrdersV2RequestOrder document "required when exchange_index is -1" but do not enforce it. A caller who passes exchange_index=-1 without market_ticker gets a server-side 400 rather than a fail-fast SDK error. This is consistent with how the rest of the SDK handles conditional spec requirements, so it is acceptable — but a model_validator on BatchCancelOrdersV2RequestOrder (parallel to DecreaseOrderV2Request._enforce_reduce_xor) would catch it earlier and give a clearer error message.

3. SubaccountBalance.exchange_index declared as required int — deserialization risk (medium severity)

subaccount_number: int
exchange_index: int   # required, no default
balance: DollarDecimal

Declaring this without a default means any API response that omits exchange_index raises ValidationError. The spec says it is required in 3.22.0, but if production is still rolling out, or a user is on an older environment, they will get a runtime crash the moment they call any endpoint that returns subaccount balances. Consider exchange_index: int | None = None until the field is confirmed in all environments. Since the model has extra = "allow", making it optional costs nothing and avoids a hard failure window.

Positives worth calling out

• RFQ method design — accept_for_rfq correctly mirrors the @overload pattern from QuotesResource.accept, and shares _build_accept_quote_body. No logic duplication.
• Path parameter sanitization — _seg(rfq_id, name='rfq_id') and _seg(quote_id, name='quote_id') on every new RFQ URL.
• confirm_for_rfq comment — the # json={} forces Content-Type note on the empty-body PUT matches the existing confirm method pattern and is the right call.
• ge=0 removal on BatchCancelOrdersV2RequestOrder.exchange_index — the comment explaining the -1 sentinel is clear.
• Contract drift test housekeeping — V1 entries fully removed from METHOD_ENDPOINT_MAP, BODY_MODEL_MAP, EXCLUSIONS, and _contract_map.py. No stale entries left behind.
• Migration doc — the V1 to V2 mapping table is exactly what users need.

Verdict

Approve with one suggested fix: the SubaccountBalance.exchange_index required-vs-optional question (point 3) is the only change that could break users silently at runtime. Everything else is clean.